Skip to content

NETOBSERV-1833: decompose TCPflags bitfield #591

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Jump to bottom
Merged
merged 1 commit into from
Sep 20, 2024
Merged

NETOBSERV-1833: decompose TCPflags bitfield #591

merged 1 commit into from
Sep 20, 2024

Conversation

jotak
Copy link
Member

@jotak jotak commented Sep 6, 2024 •
edited
Loading

Description

Now it will always shows the list of TCP flags rather than a numeric value:

image

Dependencies

n/a

Checklist

If you are not familiar with our processes or don't know what to answer in the list below, let us know in a comment: the maintainers will take care of that.

  • Is this PR backed with a JIRA ticket? If so, make sure it is written as a title prefix (in general, PRs affecting the NetObserv/Network Observability product should be backed with a JIRA ticket - especially if they bring user facing changes).
  • Does this PR require product documentation?
    • If so, make sure the JIRA epic is labelled with "documentation" and provides a description relevant for doc writers, such as use cases or scenarios. Any required step to activate or configure the feature should be documented there, such as new CRD knobs.
  • Does this PR require a product release notes entry?
    • If so, fill in "Release Note Text" in the JIRA.
  • Is there anything else the QE team should know before testing? E.g: configuration changes, environment setup, etc.
    • If so, make sure it is described in the JIRA ticket.
  • QE requirements (check 1 from the list):
    • Standard QE validation, with pre-merge tests unless stated otherwise.
    • Regression tests only (e.g. refactoring with no user-facing change).
    • No QE (e.g. trivial change with high reviewer's confidence, or per agreement with the QE team).

@openshift-ci-robot
Copy link
Collaborator

openshift-ci-robot commented Sep 6, 2024 •
edited by openshift-ci bot
Loading

@jotak: This pull request references NETOBSERV-1833 which is a valid jira issue.

Warning: The referenced jira issue has an invalid target version for the target branch this PR targets: expected the bug to target the "4.18.0" version, but no target version was set.

In response to this:

Description

Dependencies

n/a

Checklist

If you are not familiar with our processes or don't know what to answer in the list below, let us know in a comment: the maintainers will take care of that.

  • Is this PR backed with a JIRA ticket? If so, make sure it is written as a title prefix (in general, PRs affecting the NetObserv/Network Observability product should be backed with a JIRA ticket - especially if they bring user facing changes).
  • Does this PR require product documentation?
  • If so, make sure the JIRA epic is labelled with "documentation" and provides a description relevant for doc writers, such as use cases or scenarios. Any required step to activate or configure the feature should be documented there, such as new CRD knobs.
  • Does this PR require a product release notes entry?
  • If so, fill in "Release Note Text" in the JIRA.
  • Is there anything else the QE team should know before testing? E.g: configuration changes, environment setup, etc.
  • If so, make sure it is described in the JIRA ticket.
  • QE requirements (check 1 from the list):
  • Standard QE validation, with pre-merge tests unless stated otherwise.
  • Regression tests only (e.g. refactoring with no user-facing change).
  • No QE (e.g. trivial change with high reviewer's confidence, or per agreement with the QE team).

Instructions for interacting with me using PR comments are available here. If you have questions or suggestions related to my behavior, please file an issue against the openshift-eng/jira-lifecycle-plugin repository.

@jotak jotak added the ok-to-test To set manually when a PR is safe to test. Triggers image build on PR. label Sep 6, 2024
@openshift-ci OpenShift CI
Copy link

openshift-ci bot commented Sep 6, 2024

[APPROVALNOTIFIER] This PR is NOT APPROVED

This pull-request has been approved by:
Once this PR has been reviewed and has the lgtm label, please ask for approval from jotak. For more information see the Kubernetes Code Review Process.

The full list of commands accepted by this bot can be found here.

Needs approval from an approver in each of these files:

Approvers can indicate their approval by writing /approve in a comment
Approvers can cancel approval by writing /approve cancel in a comment

@github-actions GitHub Actions
Copy link

github-actions bot commented Sep 6, 2024

New image:
quay.io/netobserv/network-observability-console-plugin:6e07bcf

It will expire after two weeks.

To deploy this build, run from the operator repo, assuming the operator is running:

USER=netobserv VERSION=6e07bcf make set-plugin-image

@openshift-ci-robot
Copy link
Collaborator

openshift-ci-robot commented Sep 6, 2024 •
edited by openshift-ci bot
Loading

@jotak: This pull request references NETOBSERV-1833 which is a valid jira issue.

Warning: The referenced jira issue has an invalid target version for the target branch this PR targets: expected the bug to target the "4.18.0" version, but no target version was set.

In response to this:

Description

Now it will always shows the list of TCP flags rather than a numeric value:

image

Dependencies

n/a

Checklist

If you are not familiar with our processes or don't know what to answer in the list below, let us know in a comment: the maintainers will take care of that.

  • Is this PR backed with a JIRA ticket? If so, make sure it is written as a title prefix (in general, PRs affecting the NetObserv/Network Observability product should be backed with a JIRA ticket - especially if they bring user facing changes).
  • Does this PR require product documentation?
  • If so, make sure the JIRA epic is labelled with "documentation" and provides a description relevant for doc writers, such as use cases or scenarios. Any required step to activate or configure the feature should be documented there, such as new CRD knobs.
  • Does this PR require a product release notes entry?
  • If so, fill in "Release Note Text" in the JIRA.
  • Is there anything else the QE team should know before testing? E.g: configuration changes, environment setup, etc.
  • If so, make sure it is described in the JIRA ticket.
  • QE requirements (check 1 from the list):
  • Standard QE validation, with pre-merge tests unless stated otherwise.
  • Regression tests only (e.g. refactoring with no user-facing change).
  • No QE (e.g. trivial change with high reviewer's confidence, or per agreement with the QE team).

Instructions for interacting with me using PR comments are available here. If you have questions or suggestions related to my behavior, please file an issue against the openshift-eng/jira-lifecycle-plugin repository.

@github-actions github-actions bot removed the ok-to-test To set manually when a PR is safe to test. Triggers image build on PR. label Sep 6, 2024
@jotak jotak added the ok-to-test To set manually when a PR is safe to test. Triggers image build on PR. label Sep 6, 2024
@github-actions GitHub Actions
Copy link

github-actions bot commented Sep 6, 2024

New image:
quay.io/netobserv/network-observability-console-plugin:d6622c0

It will expire after two weeks.

To deploy this build, run from the operator repo, assuming the operator is running:

USER=netobserv VERSION=d6622c0 make set-plugin-image

@codecov Codecov
Copy link

codecov bot commented Sep 6, 2024 •
edited
Loading

Codecov Report

Attention: Patch coverage is 41.66667% with 14 lines in your changes missing coverage. Please review.

Project coverage is 56.27%. Comparing base (347afd9) to head (941ba1b).
Report is 7 commits behind head on main.

Files with missing lines Patch % Lines
web/src/components/drawer/record/record-field.tsx 8.33% 11 Missing ⚠️
web/src/utils/filter-options.ts 33.33% 2 Missing ⚠️
web/src/utils/tcp-flags.ts 88.88% 1 Missing ⚠️
Additional details and impacted files 
@@            Coverage Diff             @@
##             main     #591      +/-   ##
==========================================
- Coverage   56.29%   56.27%   -0.03%     
==========================================
  Files         190      190              
  Lines        9289     9296       +7     
  Branches     1197     1200       +3     
==========================================
+ Hits         5229     5231       +2     
- Misses       3691     3696       +5     
  Partials      369      369
Flag Coverage Δ
uitests 57.78% <41.66%> (-0.03%) ⬇️
unittests 52.06% <ø> (ø)

Flags with carried forward coverage won't be shown. Click here to find out more.

Files with missing lines Coverage Δ
web/src/utils/tcp-flags.ts 88.88% <88.88%> (ø)
web/src/utils/filter-options.ts 42.20% <33.33%> (ø)
web/src/components/drawer/record/record-field.tsx 49.17% <8.33%> (-1.98%) ⬇️

jpinsonneau
jpinsonneau approved these changes Sep 9, 2024
View reviewed changes
Copy link
Contributor

@jpinsonneau jpinsonneau left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM, thanks @jotak

@openshift-ci openshift-ci bot added the lgtm label Sep 9, 2024
@Amoghrd
Copy link
Member

Amoghrd commented Sep 9, 2024 •
edited
Loading

@jotak Filtering the flows with multiple TCP flags is not possible. For example if a flow has multiple flags like SYN_ACK and SYN and filter on one of them like SYN_ACK, then this flow does not show up on filtering either of those flags. PTAL at the screenshot below
Screenshot 2024-09-09 at 12 26 29 PM

Screenshot 2024-09-09 at 12 26 50 PM

@jotak
Copy link
Member Author

jotak commented Sep 13, 2024 •
edited
Loading

Hey @Amoghrd
I checked this issue and it's not a regression, it has always been like that. Doing as you suggest would require us to have bitwise operation in Loki queries (e.g. queries matching Flags % 16 == 0) , that's not something we have ever done and I'm don't even think it is possible in LogQL. So I fear this is a limitation we have to live with.

LogQL allows to do simple comparisons in line filters (https://grafana.com/docs/loki/latest/query/log_queries/#label-filter-expression) but not more complex arithmetic.

For instance it returns an error when parsing this:

curl -G -s -H 'X-Scope-OrgID:network' http://loki.netobserv.svc:3100/loki/api/v1/query  --data-urlencode 'query={app="netobserv-flowcollector"} | json | (Flags % 16) == 0'
parse error at line 1, col 49: syntax error: unexpected %!(NOVERB)

@jpinsonneau
Copy link
Contributor

We could actually have the parsing step before storage and change the TCP flags field to a string array as we do for Interfaces for example.
In that way we could filter on multiple values easilly.

WDYT ?

@jotak
Copy link
Member Author

jotak commented Sep 13, 2024

then maybe for another task, right? that would involve changes in FLP, and may have some little performance impact

@jotak
Copy link
Member Author

jotak commented Sep 20, 2024

merging before cutting branches, will have to be tested post-merge

@jotak jotak merged commit a90fd90 into netobserv:main Sep 20, 2024
12 of 13 checks passed
@Amoghrd
Copy link
Member

Amoghrd commented Sep 20, 2024

/label qe-approved

@openshift-ci openshift-ci bot added the qe-approved QE has approved this pull request label Sep 20, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

@jpinsonneau jpinsonneau jpinsonneau approved these changes

@msherif1234 msherif1234 Awaiting requested review from msherif1234

Assignees

@jpinsonneau jpinsonneau

Labels

jira/valid-reference lgtm ok-to-test To set manually when a PR is safe to test. Triggers image build on PR. qe-approved QE has approved this pull request

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

4 participants

@jotak @openshift-ci-robot @Amoghrd @jpinsonneau